AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

Findings

⚠️ Low — Potential infinite mouseup loop on re-dispatch

File: packages/pluggableWidgets/color-picker-web/src/components/ColorPicker.tsx line 188–189

Note: releaseDrag is registered on document in capture phase and immediately fires window.dispatchEvent(new MouseEvent("mouseup")). Because document is an ancestor of window in the event propagation model (capture flows top-down: window → document → ...), this synthetic window mouseup will also flow down to document and trigger the capture listener again — creating a recursive loop.

In jsdom (used in tests) this may be silently ignored, but in a real browser the capture listener on document will re-fire when the re-dispatched window event propagates down. The fix is to check whether the event being re-dispatched is already synthetic before dispatching again, or to attach the listener to the widget's own root element rather than document:

const releaseDrag = (e: MouseEvent): void => {
    // Avoid re-entering on the synthetic event we dispatch.
    if (e.target === window) {
        return;
    }
    window.dispatchEvent(new MouseEvent("mouseup"));
};
document.addEventListener("mouseup", releaseDrag, true);

⚠️ Low — jest.setup.ts is placed under src/ instead of package root

File: packages/pluggableWidgets/color-picker-web/src/jest.setup.ts

Note: The jest.config.js at the package root references <rootDir>/jest.setup.ts, which resolves to the package root — but the file is at src/jest.setup.ts. At runtime Jest resolves <rootDir> to the directory containing jest.config.js (the package root), so the path <rootDir>/jest.setup.ts will not find src/jest.setup.ts. The datagrid-web package has the same structure but its jest.setup.ts is also at src/jest.setup.ts while the config references <rootDir>/jest.setup.ts — if that package passes it is likely because the base config already points elsewhere or the rootDir is configured differently.

Verify that the setup file is found by confirming tests run with console.error suppression active. If not, move jest.setup.ts to the package root or update the path to <rootDir>/src/jest.setup.ts.

Positives

• The root-cause analysis is precise: identifying the bubble-phase vs. capture-phase ordering and Mendix dialog stopPropagation is non-obvious and well-documented in both the PR description and inline comment.
• The useEffect cleanup (return () => document.removeEventListener(...)) is correct and matches the active guard pattern — no event listener leaks.
• The effect correctly gates on hidden, so the listener is only active when the picker is visible, minimising unnecessary overhead.
• Test coverage directly mirrors the implementation: open/close lifecycle, no-attach-when-hidden, re-dispatch verification, and inline-mode all have dedicated cases.
• jest.config.js correctly extends the base config with spread (...base) and safely appends to setupFilesAfterEnv rather than replacing it.
• CHANGELOG entry follows Keep a Changelog format and is placed in the [Unreleased] section as required.

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

Findings

⚠️ Low — validateColor useEffect is missing validateColor from its dependency array

File: packages/pluggableWidgets/color-picker-web/src/components/ColorPicker.tsx line 176–180
Note: validateColor is now a stable useCallback, but the effect that calls it still omits it from the dependency array with no suppression comment — so if validateColor's identity ever changes (e.g. format or invalidFormatMessage changes before color changes) the effect will silently use a stale closure. The existing eslint disable at line 93 covers the validateColor callback itself; this effect should also either include validateColor in deps or add its own // eslint-disable-next-line react-hooks/exhaustive-deps comment explaining the deliberate omission.

useEffect(() => {
    if (color) {
        validateColor(color);
    }
    // validateColor is intentionally omitted — the effect should only re-run on color change
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [color]);

⚠️ Low — jest.setup.ts is placed inside src/ but config references <rootDir>/jest.setup.ts

File: packages/pluggableWidgets/color-picker-web/jest.config.js line 5
Note: The setup file lives at src/jest.setup.ts, not jest.setup.ts. The datagrid-web package follows the same pattern (also src/jest.setup.ts with <rootDir>/jest.setup.ts in config), which implies the base config sets rootDir to src/ — but this is an implicit assumption that is easy to get wrong. Consider verifying this assumption explicitly, or moving the setup file to the package root to make the path unambiguous. Not a blocker if tests pass.

⚠️ Low — test script changed from toolchain runner to bare jest

File: packages/pluggableWidgets/color-picker-web/package.json line 41
Note: Changing from pluggable-widgets-tools test:unit:web to jest is fine for local runs, but the toolchain runner may configure Jest differently (e.g. reporters, coverage thresholds, environment) for CI. Confirm that the new jest.config.js replicates all behaviour from pluggable-widgets-tools test:unit:web, or that CI calls jest directly anyway. Other packages that added a custom jest.config.js (e.g. datagrid-web, bar-chart-web) also switched to jest, so this appears to be an accepted pattern — just worth a double-check.

Positives

• The fix is minimal and precisely scoped: one useEffect with the listener attached only while the picker is visible, and removed on hide/unmount — no global side-effects when picker is closed.
• The capture-phase approach is the correct solution: it fires before any descendant stopPropagation() call, so it is robust regardless of which Mendix dialog or overlay intercepts the event.
• All five new unit tests are well-structured: they cover attach, dispatch, no-attach-when-hidden, remove-on-hide, and inline-mode — giving good confidence without testing react-color's internals.
• CHANGELOG entry is present and correctly placed in [Unreleased] using Keep a Changelog format.
• Wrapping validateColor in useCallback is a correctness improvement that prevents unnecessary recalculations downstream.
• Warning suppression in jest.setup.ts is narrow and well-commented — it only silences the specific defaultProps deprecation from react-color, not all console.error calls.